Skip to content

Add list of hosts with ports for setting connection #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

df530
Copy link

@df530 df530 commented Jan 11, 2022

It is possible to specify multiple hosts to connect to. It approves fault tolerance of connection because when one host isn't alive client can try to connect to another one.
The connection will be set to the first available host.

Client client(ClientOptions()
              .SetHost({
                  ClientOptions::HostPort("host1.com", 8000),
                  ClientOptions::HostPort("host2.com"), /// port is ClientOptions.port
              }));
              

When ResetConnection is called, it tries to connect to the first host. If connection failed, then it tries to connect to the next host in list, while list doesn't end up (round-robin approach). If list ends up, ResetConnection throws exception of last host.

@CLAassistant
Copy link

CLAassistant commented Jan 11, 2022

CLA assistant check
All committers have signed the CLA.

@PolyProgrammist
Copy link

Looks good to me. I think it's better to merge commits in one. And providing more information about the reasons for commit (fault tolerance) and about the implementation (round robin) in pull request description would be great

@filimonov
Copy link
Contributor

filimonov commented Jan 20, 2022

The idea/motivation is clean. Maybe it would be clearer to push that logic a bit deeper?

Here we have iteration through different IPs which can be extracted from the same domain name:

for (auto res = addr.Info(); res != nullptr; res = res->ai_next) {
SOCKET s(socket(res->ai_family, res->ai_socktype, res->ai_protocol));

Maybe we can generalize that code a bit for multiple hosts/ports cases instead of creating one more retry loop outside?

In that case, connect timeouts should work better.

Copy link
Contributor

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add proper unit-tests and fix highlighted issues.

/// List of hostnames with service ports
struct HostPort {
std::string host;
std::optional<unsigned int> port;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix: add #include <optional>
Or enable editing PR by maintainers.

@@ -510,6 +510,67 @@ int main() {
.SetCompressionMethod(CompressionMethod::LZ4));
RunTests(client);
}

{
Copy link
Contributor

@Enmk Enmk Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests instead, the sample program is not executed in CI/CD and hence your code is not checked. I suggest you to add a separate test-cases, both positive and negative in client_ut.cpp:

Suggested change
{
TEST(ClientCaseConnect, MultipleEndpoints) {
}

Comment on lines +56 to +62
struct HostPort {
std::string host;
std::optional<unsigned int> port;

explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename HostPort to Endpoint (here and everywhere else), it is a bit more clear. Also, you can significantly simplify the struct definition:

Suggested change
struct HostPort {
std::string host;
std::optional<unsigned int> port;
explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
}
};
struct Endpoint {
std::string host;
std::optional<unsigned int> port = std::nullopt;
}
};

explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
}
};
DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});
/** Set endpoints (host+port), only one is used.
* Client tries to connect to those endpoints one by one, on the round-robin basis:
* first default enpoint (set via SetHost() + SetPort()), then each of endpoints, from begin() to end(),
* the first one to establish connection is used for the rest of the session.
* If port part is not specified, default port (@see SetPort()) is used.
*/
DECLARE_FIELD(endpoints, std::vector<Endpoint>, SetEndpoints, {});

@@ -296,73 +320,100 @@ void Client::Impl::Ping() {
}

void Client::Impl::ResetConnection() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠ If there was a temporary communication problem, the client could re-connect to a different server (which I believe may be unexpected by the user).

Please make sure that client tries to reconnect to the same server first (options_.send_retries times).

Also I suggest to explicitly allow users to reset connection endpoint, maybe via separate function or ResetConnection() overload/parameter.

E.g.:

  • ResetConnection() - try reconnecting to current endpoint.
  • ResetConnectionEndpoint() - try connecting to different endpoint.

Copy link

@PolyProgrammist PolyProgrammist May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in direct messages that:
ResetConnectionEndpoint() should try to connect to different endpoints one by one only one time. If it doesn't work, throw an exception. It should be called on the client start and it should be called N times. Also the user can call it in order to connect to different endpoint.

ResetConnection() should try to reconnect to the current endpoint N times. It doesn't try to connect to other endpoints. If it doesn't work, throw an exception. So it's behaviour doesn't change

@@ -167,6 +190,7 @@ class Client::Impl {
#endif

ServerInfo server_info_;
std::optional<ClientOptions::HostPort> connected_host_port_;
};


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please modify endpoints creation to simplify the ResetConnection()

Suggested change
ClientOptions modifyClientOptions(ClientOptions opts)
{
if (!opts.host.empty())
opts.endpoints.insert(opts.endpoints.begin(), ClientOptions::Endpoint{opts.host, opts.port});
return opts;
}
Client::Impl::Impl(const ClientOptions& opts)
: options_(modifyClientOptions(opts))
{

@@ -296,73 +320,100 @@ void Client::Impl::Ping() {
}

void Client::Impl::ResetConnection() {
connected_host_port_.reset();
for (int i = -1; i < int(options_.hosts_ports.size()); ++i) {
const ClientOptions::HostPort& host_port = i == -1 ? ClientOptions::HostPort(options_.host) : options_.hosts_ports[i];
Copy link
Contributor

@Enmk Enmk Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the list of endpoints before copying it to a options_ so you don't have to do this. (see above)

@@ -196,6 +206,8 @@ class Client {

const ServerInfo& GetServerInfo() const;

const std::optional<ClientOptions::HostPort>& GetConnectedHostPort() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify in comments when this function may return nullopt.

Comment on lines +515 to +528
ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000);
Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("localhost", 8000), // wrong port
ClientOptions::HostPort("localhost", 7000), // wrong port
ClientOptions::HostPort("1127.91.2.1"), // wrong host
ClientOptions::HostPort("1127.91.2.2"), // wrong host
ClientOptions::HostPort("notlocalwronghost"), // wrong host
ClientOptions::HostPort("another_notlocalwronghost"), // wrong host
correct_host_port,
ClientOptions::HostPort("localhost", 9001), // wrong port
ClientOptions::HostPort("1127.911.2.2"), // wrong host
})
.SetPingBeforeQuery(true));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this allows simplifying client code:

Suggested change
ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000);
Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("localhost", 8000), // wrong port
ClientOptions::HostPort("localhost", 7000), // wrong port
ClientOptions::HostPort("1127.91.2.1"), // wrong host
ClientOptions::HostPort("1127.91.2.2"), // wrong host
ClientOptions::HostPort("notlocalwronghost"), // wrong host
ClientOptions::HostPort("another_notlocalwronghost"), // wrong host
correct_host_port,
ClientOptions::HostPort("localhost", 9001), // wrong port
ClientOptions::HostPort("1127.911.2.2"), // wrong host
})
.SetPingBeforeQuery(true));
ClientOptions::Endpoint correct_endpoint{"localhost", 9000};
Client client(ClientOptions()
.SetEndpoints({
{"localhost", 8000}, // wrong port
{"localhost", 7000}, // wrong port
{"1127.91.2.1"}, // wrong host
{"1127.91.2.2"}, // wrong host
{"notlocalwronghost"}, // wrong host
{"another_notlocalwronghost"}, // wrong host
{correct_endpoint},
{"localhost", 9001}, // wrong port
{"1127.911.2.2"}, // wrong host
})
.SetPingBeforeQuery(true));

Copy link
Contributor

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add proper unit-tests and fix highlighted issues.

@Enmk
Copy link
Contributor

Enmk commented May 3, 2022

@DF5HSE ping?

@PolyProgrammist
Copy link

@Enmk hi, I have just started to continue working on this pr. Hope it will be ready soon

@Enmk Enmk closed this Apr 19, 2023
@MikhailBurdukov
Copy link
Contributor

Hi!
We think this feature will be very useful for us, so I would like to complete it. Can we reopen PR or is it better to open a new one?

@MikhailBurdukov
Copy link
Contributor

@Enmk ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants